Context-aware instance manager #1352
Conversation
github-project-automation
bot
moved this to 📬 Triage
in Plugins Platform / Grafana Community
github-project-automation
bot
moved this from 📬 Triage
to 🔬 In review
in Plugins Platform / Grafana Community
| cache.OnEvicted(func(_ string, value interface{}) { | ||
| ci := value.(CachedInstance) | ||
| if disposer, valid := ci.instance.(InstanceDisposer); valid { | ||
| time.AfterFunc(disposeTTL, func() { |
There was a problem hiding this comment.
@iwysiu IIUC we may no longer need to make use of the disposeTTL here - can you confirm if my understanding is correct?
When the instanceTTL expires, the instance will be inaccessible via Get() but still exist in memory. When the cleanup interval comes around, as long as that happens a reasonable amount of time after the instanceTTL, then we don't need to worry about anyone have a hold of the instance so we can dispose right away. Also once the instanceTTL expires, there's no way to revive it.
There was a problem hiding this comment.
I think we should be ok to remove it, but do we think there's a possibility of the following/do we care if it happens?
- instance fetched
- instance expired
- instance cleared (theoretically possible if it just happens at a regular pace?)
- instance used
I think that's the only possible problem seres of events
There was a problem hiding this comment.
In this scenario - are you imagining that in order for 4. to occur, there would have to be a long-lived piece of code that is running where the instance is being used?
The instance is fetched every time a query is made (QueryData, CheckHealth, etc.), then expires after no usage for 1hr. It exists in memory for another hour before being disposed, but cannot be fetched again in that timeframe since it's expired according to the cache. The only way I see 4 occurring is if some code still has access to the instance elsewhere?
An alternative to #1230 that implements TTL for datasource instances, but decides that logic at runtime via a feature toggle